Skip to content

MPT-18457 MPT API Client - Missing type for in RQL query#219

Merged
jentyk merged 1 commit intomainfrom
fix/MPT-18457
Feb 27, 2026
Merged

MPT-18457 MPT API Client - Missing type for in RQL query#219
jentyk merged 1 commit intomainfrom
fix/MPT-18457

Conversation

@jentyk
Copy link
Contributor

@jentyk jentyk commented Feb 26, 2026

Closes MPT-18457

  • Import Iterable from collections.abc and add Iterable to the public QueryValue type alias.
  • Accept iterable-backed values (e.g., list/tuple/set and other Iterable) as QueryValue for RQL encoding.
  • rql_encode: for non-list operators, treat QueryValue/RQLValue/RQLProperty via query_value_str; for list operators, join list/tuple/set elements with commas to build list-style RQL expressions.
  • parse_kwargs: continue to map list operators (e.g., in) to op(field,(...)) using rql_encode for element encoding.

@jentyk jentyk requested a review from a team as a code owner February 26, 2026 11:48
@coderabbitai
Copy link

coderabbitai bot commented Feb 26, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR updates mpt_api_client/rql/query_builder.py by importing Iterable from collections.abc and expanding the public QueryValue type alias to include Iterable in addition to str, bool, date, datetime, and Numeric.

Changes

Cohort / File(s) Summary
Query Value Type Expansion
mpt_api_client/rql/query_builder.py
Added from collections.abc import Iterable and extended the exported QueryValue type alias from `str

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Test Coverage Required ⚠️ Warning PR modifies code in mpt_api_client/rql/query_builder.py to extend QueryValue type for Iterable support, but no test files were updated despite existing test coverage. Add or update tests in tests/unit/rql/query_builder/ to cover the new Iterable type support in QueryValue.
✅ Passed checks (2 passed)
Check name Status Explanation
Jira Issue Key In Title ✅ Passed PR title contains exactly one Jira issue key in the required format MPT-XXXX: MPT-18457.
Single Commit Required ✅ Passed The pull request contains exactly one commit (344faf2 fix(rql): add support for iterable of LiteralString in query value definition), satisfying the single commit requirement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@jentyk jentyk force-pushed the fix/MPT-18457 branch 2 times, most recently from efa8423 to d11599d Compare February 26, 2026 11:53
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@mpt_api_client/rql/query_builder.py`:
- Line 10: Remove Iterable from the broad QueryValue alias and split it into a
scalar alias and a collection alias: define QueryScalar = str | bool | dt.date |
dt.datetime | Numeric and a separate QueryCollection type (e.g. list | tuple |
set of QueryScalar or a Sequence/Collection[...] from collections.abc) and
replace usages of QueryValue accordingly; keep QueryCollection for the "in" /
list operators and use QueryScalar for eq/ne/lt/... paths, and update the
runtime checks around the variable value and op (the branches that reference
constants.LIST and isinstance(value, list | tuple | set)) to validate against
QueryCollection instead of the old Iterable alias so scalar operators no longer
accept arbitrary iterables and the WPS221 lint error is resolved.

ℹ️ Review info

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0c4a94 and 006fc3d.

📒 Files selected for processing (1)
  • mpt_api_client/rql/query_builder.py

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
mpt_api_client/rql/query_builder.py (1)

2-10: ⚠️ Potential issue | 🔴 Critical

QueryValue now causes runtime TypeError in isinstance paths; split scalar vs list types.

Line 10 adds Iterable[str] into QueryValue, and QueryValue is used in runtime isinstance(...) checks (Line 105, Line 143). That raises TypeError: isinstance() argument 2 cannot be a parameterized generic, which matches the failing pipeline traces.

💡 Proposed fix
 from decimal import Decimal
 from typing import Any, Self, override

 Numeric = int | float | Decimal
-
-QueryValue = str | bool | dt.date | dt.datetime | Numeric | Iterable[str]  # noqa:WPS221
+QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric
+QueryListValue = list[QueryScalarValue] | tuple[QueryScalarValue, ...] | set[QueryScalarValue]
+QueryValue = QueryScalarValue
+QueryArgumentValue = QueryScalarValue | QueryListValue

@@
-def parse_kwargs(query_dict: dict[str, QueryValue]) -> list[str]:  # noqa: WPS231
+def parse_kwargs(query_dict: dict[str, QueryArgumentValue]) -> list[str]:  # noqa: WPS231

@@
-        **kwargs: QueryValue,
+        **kwargs: QueryArgumentValue,

@@
-def query_value_str(value: Any) -> str:
+def query_value_str(value: Any) -> str:
     """Converts a value to string for use in RQL queries."""
-    if isinstance(value, QueryValue):
+    if isinstance(value, QueryScalarValue):
         value = RQLValue(value)

@@
 def rql_encode(op: str, value: Any) -> str:
@@
-    if op not in constants.LIST and isinstance(value, QueryValue | RQLValue | RQLProperty):
+    if op not in constants.LIST and isinstance(value, QueryScalarValue | RQLValue | RQLProperty):
         return query_value_str(value)
#!/bin/bash
set -euo pipefail

# Verify root cause exists in current tree (read-only checks).
rg -nP '^\s*QueryValue\s*=.*Iterable\[[^]]+\]' mpt_api_client/rql/query_builder.py
rg -nP '\bisinstance\([^)]*,\s*QueryValue(\s*\||\s*\))' mpt_api_client/rql/query_builder.py
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/rql/query_builder.py` around lines 2 - 10, QueryValue
currently includes a parameterized generic (Iterable[str]) which causes
TypeError when used in runtime isinstance checks; split the alias into a scalar
union and a non-parameterized iterable alias, then update the isinstance checks
to use those concrete symbols. Concretely: replace QueryValue with two aliases
such as QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric and
QueryIterableValue = Iterable (or Iterable[Any]/collections.abc.Iterable without
type parameters), or define QueryListValue = Iterable[str] only for type hints,
and update any runtime isinstance(...) use (the checks around the previous
QueryValue usage) to test against QueryScalarValue's concrete types (e.g., (str,
bool, dt.date, dt.datetime, int, float, Decimal)) or check isinstance(value,
collections.abc.Iterable) for lists; keep the type-only parameterized
Iterable[str] for typing annotations but never pass a parameterized generic into
isinstance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 2-10: QueryValue currently includes a parameterized generic
(Iterable[str]) which causes TypeError when used in runtime isinstance checks;
split the alias into a scalar union and a non-parameterized iterable alias, then
update the isinstance checks to use those concrete symbols. Concretely: replace
QueryValue with two aliases such as QueryScalarValue = str | bool | dt.date |
dt.datetime | Numeric and QueryIterableValue = Iterable (or
Iterable[Any]/collections.abc.Iterable without type parameters), or define
QueryListValue = Iterable[str] only for type hints, and update any runtime
isinstance(...) use (the checks around the previous QueryValue usage) to test
against QueryScalarValue's concrete types (e.g., (str, bool, dt.date,
dt.datetime, int, float, Decimal)) or check isinstance(value,
collections.abc.Iterable) for lists; keep the type-only parameterized
Iterable[str] for typing annotations but never pass a parameterized generic into
isinstance.

ℹ️ Review info

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 006fc3d and d11599d.

📒 Files selected for processing (1)
  • mpt_api_client/rql/query_builder.py

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
mpt_api_client/rql/query_builder.py (1)

2-10: ⚠️ Potential issue | 🔴 Critical

Pipeline failure: mypy requires type parameters for Iterable.

The CI is failing with Missing type parameters for generic type 'Iterable' [type-arg]. However, adding a type parameter (e.g., Iterable[str]) will cause a runtime TypeError on line 143 because isinstance() cannot accept parameterized generics.

The previous review comment's suggested fix remains the correct approach—split into scalar and collection aliases:

💡 Recommended refactor
-from collections.abc import Iterable
 from decimal import Decimal
 from typing import Any, Self, override

 Numeric = int | float | Decimal
-
-QueryValue = str | bool | dt.date | dt.datetime | Numeric | Iterable  # noqa:WPS221
+QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric
+QueryListValue = list[QueryScalarValue] | tuple[QueryScalarValue, ...] | set[QueryScalarValue]
+QueryValue = QueryScalarValue  # For scalar operators (eq, ne, lt, etc.)
+QueryArgumentValue = QueryScalarValue | QueryListValue  # For kwargs accepting both

Then update function signatures and runtime checks accordingly:

  • parse_kwargs: use QueryArgumentValue
  • RQLQuery.__init__(**kwargs): use QueryArgumentValue
  • Line 143: use QueryScalarValue in the isinstance check
  • Line 145: keep list | tuple | set for the collection branch

,

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/rql/query_builder.py` around lines 2 - 10, Define separate
scalar and collection aliases instead of using a parameterless Iterable: add
QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric and
QueryCollectionValue = list | tuple | set and then define QueryArgumentValue =
QueryScalarValue | QueryCollectionValue; update function/type annotations to use
QueryArgumentValue (e.g., parse_kwargs and RQLQuery.__init__), change the
runtime isinstance check that currently uses Iterable to check against
QueryScalarValue (for scalars) and keep the collection branch using list | tuple
| set so isinstance() calls remain valid at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 2-10: Define separate scalar and collection aliases instead of
using a parameterless Iterable: add QueryScalarValue = str | bool | dt.date |
dt.datetime | Numeric and QueryCollectionValue = list | tuple | set and then
define QueryArgumentValue = QueryScalarValue | QueryCollectionValue; update
function/type annotations to use QueryArgumentValue (e.g., parse_kwargs and
RQLQuery.__init__), change the runtime isinstance check that currently uses
Iterable to check against QueryScalarValue (for scalars) and keep the collection
branch using list | tuple | set so isinstance() calls remain valid at runtime.

ℹ️ Review info

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d11599d and ee54717.

📒 Files selected for processing (1)
  • mpt_api_client/rql/query_builder.py

@jentyk jentyk force-pushed the fix/MPT-18457 branch 2 times, most recently from d881b4f to 344faf2 Compare February 26, 2026 12:12
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
mpt_api_client/rql/query_builder.py (1)

10-10: ⚠️ Potential issue | 🟠 Major

Unresolved from prior review: split scalar vs collection aliases instead of widening QueryValue.

Line 10 still broadens QueryValue to Iterable, so scalar operators (eq/ne/lt/...) now accept list/tuple/set and serialize them instead of failing fast. That changes behavior beyond the in/out use case.

Proposed refactor
-from collections.abc import Iterable
 from decimal import Decimal
 from typing import Any, Self, override

 Numeric = int | float | Decimal
-
-QueryValue = str | bool | dt.date | dt.datetime | Numeric | Iterable  # type: ignore[type-arg] # noqa: WPS221
+QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric
+QueryListValue = list[QueryScalarValue] | tuple[QueryScalarValue, ...] | set[QueryScalarValue]
+QueryValue = QueryScalarValue
+QueryArgumentValue = QueryScalarValue | QueryListValue
-def parse_kwargs(query_dict: dict[str, QueryValue]) -> list[str]:  # noqa: WPS231
+def parse_kwargs(query_dict: dict[str, QueryArgumentValue]) -> list[str]:  # noqa: WPS231
     def __init__(  # noqa: WPS211
         self,
         namespace_: str | None = None,  # noqa: WPS120
-        **kwargs: QueryValue,
+        **kwargs: QueryArgumentValue,
     ) -> None:
#!/bin/bash
set -euo pipefail

# Verify alias split and where each alias is used
rg -n "^(Numeric|QueryScalarValue|QueryListValue|QueryArgumentValue|QueryValue)\s*=" mpt_api_client/rql/query_builder.py

# Verify scalar operators still accept scalar-only alias
rg -n "def (eq|ne|lt|le|gt|ge|like|ilike)\(self, value:" mpt_api_client/rql/query_builder.py

# Verify kwargs/parser accept scalar + collection arguments
rg -n "def parse_kwargs|def __init__\(" mpt_api_client/rql/query_builder.py

# Verify runtime branch separation remains explicit
rg -n "if op not in constants\.LIST|if op in constants\.LIST" mpt_api_client/rql/query_builder.py

As per coding guidelines, "Typing and type aliases... project config enforces strict typing and mypy checks; ensure proper imports and type hints conform to strict settings."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/rql/query_builder.py` at line 10, The QueryValue alias was
widened to include Iterable which lets scalar operators accept collections;
split the alias into explicit QueryScalarValue (str | bool | dt.date |
dt.datetime | Numeric) and QueryListValue (Iterable[QueryScalarValue]) and use a
union QueryArgumentValue = QueryScalarValue | QueryListValue (or keep QueryValue
as the union) so scalar-only methods (eq, ne, lt, le, gt, ge, like, ilike)
accept only QueryScalarValue signatures while in/out and list-handling code uses
QueryListValue; update type hints in the eq/ne/... method signatures,
parse_kwargs and __init__ to accept the correct aliases, and ensure runtime
branches still check operators against constants.LIST where needed (e.g., in
serialize/parse code) to preserve behavior and satisfy mypy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Line 10: The QueryValue alias was widened to include Iterable which lets
scalar operators accept collections; split the alias into explicit
QueryScalarValue (str | bool | dt.date | dt.datetime | Numeric) and
QueryListValue (Iterable[QueryScalarValue]) and use a union QueryArgumentValue =
QueryScalarValue | QueryListValue (or keep QueryValue as the union) so
scalar-only methods (eq, ne, lt, le, gt, ge, like, ilike) accept only
QueryScalarValue signatures while in/out and list-handling code uses
QueryListValue; update type hints in the eq/ne/... method signatures,
parse_kwargs and __init__ to accept the correct aliases, and ensure runtime
branches still check operators against constants.LIST where needed (e.g., in
serialize/parse code) to preserve behavior and satisfy mypy.

ℹ️ Review info

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d881b4f and 344faf2.

📒 Files selected for processing (1)
  • mpt_api_client/rql/query_builder.py

Copy link
Contributor

@albertsola albertsola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add support for Iterable in RQLValue.__str__ also would be good to have some unit tests for passing an iterable value.

@albertsola
Copy link
Contributor

Currently we do not expect QueryValue to be a list, dict or Iterable, in that case (for example when we check if a value is IN a list we do it like the example below.

Would it be better to change it to: from def in_(self, value: list[QueryValue]) -> Self: to def in_(self, value: Iterable[QueryValue]) -> Self: ?

    def in_(self, value: list[QueryValue]) -> Self:
        """Check if the `RQLQuery` objects refers it is in the list of values.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.in_(['value1', 'value2'])
        """
        return self._list("in", value)

    def oneof(self, value: list[QueryValue]) -> Self:
        """
        Apply the `in` operator to the field this `RQLQuery` object refers to.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.oneof(['value1', 'value2'])
        """
        return self._list("in", value)

@albertsola
Copy link
Contributor

Also:

Is this not what you want?

RQLQuery(“product_id__in”: ['PRD-1', 'PRD-2'])

or

RQLQuery("product.id").in_(['PRD-1', 'PRD-2'])

?

@jentyk
Copy link
Contributor Author

jentyk commented Feb 27, 2026

Currently we do not expect QueryValue to be a list, dict or Iterable, in that case (for example when we check if a value is IN a list we do it like the example below.

Would it be better to change it to: from def in_(self, value: list[QueryValue]) -> Self: to def in_(self, value: Iterable[QueryValue]) -> Self: ?

    def in_(self, value: list[QueryValue]) -> Self:
        """Check if the `RQLQuery` objects refers it is in the list of values.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.in_(['value1', 'value2'])
        """
        return self._list("in", value)

    def oneof(self, value: list[QueryValue]) -> Self:
        """
        Apply the `in` operator to the field this `RQLQuery` object refers to.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.oneof(['value1', 'value2'])
        """
        return self._list("in", value)

I didn't notice this. Thanks for spotting. I think the issue I found was coming from a different place. Have a look at the new version.

@jentyk jentyk requested review from albertsola and d3rky February 27, 2026 11:16
@albertsola
Copy link
Contributor

albertsola commented Feb 27, 2026

Currently we do not expect QueryValue to be a list, dict or Iterable, in that case (for example when we check if a value is IN a list we do it like the example below.
Would it be better to change it to: from def in_(self, value: list[QueryValue]) -> Self: to def in_(self, value: Iterable[QueryValue]) -> Self: ?

    def in_(self, value: list[QueryValue]) -> Self:
        """Check if the `RQLQuery` objects refers it is in the list of values.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.in_(['value1', 'value2'])
        """
        return self._list("in", value)

    def oneof(self, value: list[QueryValue]) -> Self:
        """
        Apply the `in` operator to the field this `RQLQuery` object refers to.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.oneof(['value1', 'value2'])
        """
        return self._list("in", value)

I didn't notice this. Thanks for spotting. I think the issue I found was coming from a different place. Have a look at the new version.

Can you provide an example in a form of a unit test that validate those changes and covers a potential user case?

Currently QueryValue is always expected to be a Scalar. I do not comprehend why we need to pass an Iterator where we expect a Scalar.

Thank you so much

Copy link
Contributor

@albertsola albertsola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jentyk
Copy link
Contributor Author

jentyk commented Feb 27, 2026

Currently we do not expect QueryValue to be a list, dict or Iterable, in that case (for example when we check if a value is IN a list we do it like the example below.
Would it be better to change it to: from def in_(self, value: list[QueryValue]) -> Self: to def in_(self, value: Iterable[QueryValue]) -> Self: ?

    def in_(self, value: list[QueryValue]) -> Self:
        """Check if the `RQLQuery` objects refers it is in the list of values.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.in_(['value1', 'value2'])
        """
        return self._list("in", value)

    def oneof(self, value: list[QueryValue]) -> Self:
        """
        Apply the `in` operator to the field this `RQLQuery` object refers to.

        Args:
            value: The list of values to which compare the field.

        Examples:
            RQLQuery().field.oneof(['value1', 'value2'])
        """
        return self._list("in", value)

I didn't notice this. Thanks for spotting. I think the issue I found was coming from a different place. Have a look at the new version.

Can you provide an example in a form of a unit test that validate those changes and covers a potential user case?

Currently QueryValue is always expected to be a Scalar. I do not comprehend why we need to pass an Iterator where we expect a Scalar.

Thank you so much

I added a test, have a look. The test is just an example and should be deleted I do not see a point of keeping it

Copy link
Contributor

@albertsola albertsola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I see the problem you are solving!

@sonarqubecloud
Copy link

@jentyk jentyk merged commit 878b327 into main Feb 27, 2026
5 checks passed
@jentyk jentyk deleted the fix/MPT-18457 branch February 27, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants